fix(icom): Restore durable Icom memory ownership and synced recall - #5328

Issue fit

There is no fixes/closes #NNNN in the commit (9a17060 Restore durable Icom memory ownership (Principle XI)), so I reviewed against the PR's own stated intent: make AetherSDR's shared client database the working memory store for every Icom, turn 1A 00 reads into an ingestion path rather than an ownership handover, and have repeated syncs update the same row instead of duplicating it. The first two are delivered cleanly and are, in my read, the right architectural call — persistsMemories was doing double duty as "who owns the store" and "can this radio be read", and splitting canRefreshMemories out of it is a genuine improvement over the previous profile-gated persistsMemories. The third claim does not hold in the one case that matters most; see Blocker 1.

Per GOVERNANCE.md this is close to an architectural change (it reverses which store owns Icom memories and bumps a persisted schema version), and it arrives with no linked issue or RFC. That is a maintainer call, not something I'd block on by itself, but it is worth noting that the capability semantics being changed here were themselves ratified through RFC #4603.

Scope

Nothing here is unrelated to the stated change. MemoryDelta::importSource/importKey is new cross-seam surface (backend → model), but it is internal C++, not a protocol verb or a settings key, and it is the mechanism the change requires. The persisted JSON schema bump is new durable surface — flagged below rather than waved through.

I did not find a false self-certification: no CHANGELOG.md entry (correct), no unrelated files, no formatting churn, no removed guards. Reading the - lines specifically: the only deleted logic is c.persistsMemories = memory != nullptr and c.canRefreshMemories = c.persistsMemories, both replaced deliberately; no comment naming a fixed symptom was dropped.

Blockers

1. The import identity is an IP address for every Icom, so a DHCP lease change permanently duplicates the whole channel set. (inline: IcomCivBackend.cpp:711)

The new comment says "Prefer the discovery identity so repeated syncs from the same radio update their rows; retain a deterministic endpoint fallback for a manually-entered radio that reports no serial." I could not reproduce a case where the first branch is ever taken. Icom has no discovery — ConnectionPanel.cpp:2739-2741 is the only producer of an Icom RadioInfo, and it always synthesizes info.serial = "icom:<ip>" (or "icom:<ip>:<port>" off the default base port), with its own comment saying "No discovery means no MAC and no reported serial, so the host is the only stable identity this radio has for us." That flows to req.serial at RadioModel.cpp:2436/:3456, so request.serial.trimmed() is never empty and the request.host fallback is dead code.

Net effect: m_memoryImportSource is always "icom:icom:<ip>". Failure scenario — an IC-9700 on DHCP, synced at 192.168.1.50, gets a new lease as 192.168.1.71, operator hits Sync again: importedSlot() matches nothing, applyMemoryChanges takes the targetIndex < 0 branch and calls memory create for every occupied channel, up to 297 fresh rows land beside the 297 already there. Same on the same radio reached via a hostname vs. an IP, or on the default base port vs. a custom one (different synthesized serial string). Nothing prunes the old rows: the d.removed path only forgets rows matching the current source. The ghosts are permanent, live in the durable settings-backed document that settings backup/export covers, and are visually indistinguishable in the dialog from the operator's own memories.

This falsifies the PR's and the design doc's central claim ("Repeated syncs update rows from the same radio/channel rather than duplicating them") — it is true only while the IP never changes, which is not a property a DHCP client has.

In fairness to the author: RadioModel::settingsScope() already keys per-radio state off this same IP-derived serial, so the PR is consistent with existing practice. The consequence differs, though. For settings, an address change means "per-radio settings reset" — annoying and self-limiting. Here it means the shared, backed-up memory database silently doubles and mixes ghost rows into the operator's list, with hand-deletion the only cleanup.

Fixes I'd consider, in preference order: (a) key importSource on something the radio actually reports — the CI-V address plus m_model->name is already decoded by adoptReportedCivAddress() and is address-independent; (b) keep the IP key but, at the end of a successful, complete refresh, reconcile: prune rows whose importSource names this same model but a stale endpoint; (c) at minimum, document the duplication and give the dialog a way to select all rows from one importSource. Option (a) also makes the "discovery identity" comment true.

2. On an IC-705, Sync now silently does nothing for most filter-combo selections. (inline: MemoryDialog.cpp:1484)

IcomCivBackend::refreshMemories() resolves groupName against the profile's group names and, when memory.requiresGroupSelection (the IC-705), bails with a bare return if it did not match (IcomCivBackend.cpp:1097-1099) — no signal, no toast, no memoryRefreshStarted. Before this PR the Icom combo contained only group names, so the sole way to hit that return was leaving it on "All Memories". This diff moves Icom onto the else branch, where the combo is relabelled "Profile:" and now carries global profiles + TX profiles + radio groups in one flat sorted list, and MemoryDialog.cpp:467 passes whatever is selected straight to refreshMemories().

Failure scenario: IC-705 operator with any global or TX profile defined, combo left on (or sorted to) a profile name, clicks Sync — the button appears to work and nothing whatsoever happens, with no way to tell that from a radio that answered with zero occupied channels. The blast radius is bigger than the old one-value case, and it is introduced by this hunk. Either gate/label the Sync affordance on the selection being a member of capabilities.memoryGroups, or have refreshMemories() report the refusal rather than returning silently.

Nits (non-blocking)

• The schema bump is a one-way door and is not called out. kFormatVersion 1→2 is backward-compatible on read (parse() only rejects version > kFormatVersion, so v1 documents load fine, and every new field reads through a defaulted accessor). It is not forward-compatible: LocalMemoryBank::load()'s row-version guard and parse()'s body check both make a v2 document read-only on any older build, so a downgrade after one sync leaves the operator's whole bank refusing edits with a "newer than this build" warning. That is the designed behaviour and the bump is defensible — dropping the new fields on an old build's write-back would be worse — but it deserves a sentence in the PR body.
• Dead code the change leaves behind. With persistsMemories now false on every Icom and Flex the only backend setting it true, usesNativeMemorySchema (MemoryDialog.cpp:749-750 and :1456-1457) is unconditionally false, so ~45 lines of native-column layout and the entire RadioCapabilities::memoryGroupColumnTitle field (its only reader is MemoryDialog.cpp:757) are now unreachable. Likewise the whole !usesLocalMemoryBank() read-only-radio-store branch in tryMemoryCommand (RadioModel.cpp:7797-7827): reaching it needs persistsMemories, which only Flex has, and Flex sets canWriteMemories/canApplyMemories true so it falls straight to return nullopt. Not wrong, but it is a subsystem with no users, and the caps doc's canApplyMemories row still describes that path's behaviour as live ("applies recallable cached fields through the existing neutral slice setters") — worth either deleting or marking as reserved-for-future while the docs are being de-drifted anyway.
• Lookup key is not normalized the way the stored key is. applyMemoryChanges stores sanitize(*d.importSource) / sanitize(*d.importKey) (RadioModel.cpp:8078-8079) but importedSlot() is called with the raw values (:8027). sanitizeText strips C0 and 0x7f, which today's values never contain, so this is latent — but if it ever fires, the symptom is a fresh row on every sync forever, i.e. Blocker 1's failure mode with a harder-to-find cause. Cheapest fix is to sanitize once before both uses.
• CodeGuard's two CG-PATH-001 hits are false positives — MemoryDialog.cpp:1194 and :1420 are QProgressDialog::setLabelText format strings ("Importing %1 of %2 memories…" / "Deleting %1 of %2 memories…"). No filesystem path is involved and neither line is in this diff. Dropping them.

What I tried to break

• "Repeated syncs update rows rather than duplicating them." Broke it — Blocker 1. Traced importSource back through RadioConnectRequest.serial to its only producer to confirm the host fallback is unreachable and the identity is always endpoint-derived.
• "A radio's channel 1 cannot overwrite the operator's client slot 1." This one held, and I pushed on it. allocateSlot() picks the lowest free index in the bank, publishLocalMemories() seeds m_memories from the bank on the connect edge via onConnected → syncMemoryStoreForSession, and I confirmed onConnected is wired to IRadioBackend::connected (RadioModel.cpp:1523) and not Flex-only — so the two maps are in sync before any create, and no aliasing is possible. Within one sync the ordering is safe too: record() writes the provenance pair before the next channel's importedSlot() scan.
• Radio-swap and disconnect lifecycle. Checked Flex→Icom and Icom→Flex. onDisconnected clears m_memories, flushes the bank, and re-publishes (RadioModel.cpp:7053-7066), and syncMemoryStoreForSession clears before a radio-owned dump — so no leakage of Flex slots into the bank-published cache, in either direction. The m_sessionRadioOwnsMemories latch behaves correctly across a link blip on the new Icom path.
• Failure paths. An unreadable/unwritable bank: memory create rejects, the new code logs via qCWarning(lcProtocol) and returns without touching the cache — correct. d.removed for a channel never imported: targetIndex < 0, early return, no spurious memoryRemoved — correct. Non-occupied channels during an IC-705 group sweep behave.
• Recall after import. Confirmed MemoryEntry::recallable defaults to true, so manual/CSV rows still tune on an Icom now that memory apply routes through the local bank into recallCachedMemory() instead of the old read-only branch; and that the newly-persisted nativeFilter/dataMode/rxToneValue/DTCS fields are exactly the ones recallCachedMemory() feeds to applyMemoryRecallDetails(), so a synced repeater channel survives a restart intact. This is the part of the PR I'd most want kept.
• Filter/group namespace collision. populateTable filters on m.group, and the backend sets delta.group = memoryGroupName(...), so the new combo entries do match imported rows — the added loop is necessary, not cosmetic, and it is in the live else branch rather than the dead one. It did surface Blocker 2.
• Attacked the tests. radio_capability_gating_test additions genuinely fail against the unfixed code (old persistsMemories was true for all three profiles). local_memory_bank_test's reopen case is stronger than it looks — the second bank uses a different file path, so it proves the round trip goes through the shared settings document, not the legacy file. But nothing covers RadioModel::applyMemoryChanges's import routing, which is the actual headline behaviour: no test asserts that a radio channel lands in a fresh slot instead of slot 1, that a second sync reuses the row, or that an unoccupied channel forgets it. importedSlot() being correct is necessary, not sufficient. grep over tests/ shows MemoryDelta appears only in aetherd_residual_decode_test.cpp, so there is no existing harness to extend — which is probably why, but it is the test that would have caught Blocker 1.
• What I could not check: CI is green on all four checks for 9a17060 (build, check-windows, check-macos, Static checks), but ctest in ci.yml is -R-filtered, so green does not mean the ~240-test suite passed. I have no build and no GUI here — every finding above is reasoned from the code at /tmp/aetherclaude/pr-5328, not reproduced at runtime. In particular I could not observe an actual IC-705/IC-9700 sync, so Blockers 1 and 2 are read off the call graph rather than watched happening.

Recommendation

Request changes, narrowly. The architecture is right and I'd like to see it land: separating "who owns the working store" from "can this radio be read as an import source" is a real improvement, the durable-recall field set is the part operators will feel, and the lifecycle handling around connect/disconnect/radio-swap survived everything I threw at it. But Blocker 1 makes the PR's own headline guarantee conditional on a stable IP, and the failure is silent, permanent, and lands in a backed-up document — that wants fixing before merge, ideally by keying the identity on the CI-V-reported model instead of the endpoint. Blocker 2 is a smaller, self-contained fix in the same area. Concrete next step: re-key m_memoryImportSource, add one regression test over applyMemoryChanges covering create-then-resync-then-remove, and either gate the Sync button on a real group selection or make refreshMemories say why it refused. Blocker 1's fix option (a) vs (b) is worth a maintainer opinion first, since (b) implies a pruning policy nobody has ruled on.

Thanks for the unusually careful doc and comment work here — the radio-capabilities-map.md rewrite of the persistsMemories row is clearer than what it replaced, and the reasoning comments in applyMemoryChanges made this reviewable.

_{🤖 aethersdr-agent · cost: $10.4712 · model: claude-opus-5}

Issue fit

#5327's regression (durable database hidden, Tune/Import broken, transient sync rows) is genuinely fixed, and the live automation-bridge validation on the IC-7300MK2 — 221 rows across restart, idempotent double-Sync, panadapter-spot recall — is exactly the right kind of evidence and covers what it claims. The provenance design (icom:<guid> + native <group>:<channel> key, all-zero GUID refused) is sound. The problems are at the edges the live run's own coverage-limits section honestly lists as untested — and several of them are the sharp edges.

Scope

All files map to the issue. One out-of-band item: none — the diff is clean. Preflight: no sockets, no fake peers; every new test is socket-free.

Blockers

1. Split/RPS recall is unevidenced, and recalling one pushes wrong state at a live radio (inline at the codec). The deleted rule was recallable = mode && !split && duplex != 3; the new rule keeps only the mode test, justified by a comment claiming "the local database already has a complete, neutral RX frequency" — but IcomMemoryChannel has no field for the second frequency block (it is parsed for nothing), and duplex == 3 falls through the backend's default: arm to offsetDir = "simplex". Tune on an RPS/split row therefore sets the slice to the first-block frequency and pushes DUP=simplex to the radio — clearing the stored split and, on transmit, keying on the wrong frequency. The PR body's own coverage-limits section says split/reverse-split recall was never live-tested, and the changed test asserts the flag, not that the recalled frequency is the RX side. Until the second block is decoded (or its semantics evidenced per the repo's docs/data/ precedent), duplex == 3 and split rows should stay display-only.

2. Re-Sync silently destroys operator edits on imported rows (inline at the stamp). delta.owner/delta.group/delta.name are unconditionally re-stamped on every pass, and applyMemoryChanges applies any present field to the matched row — so an operator who annotates Owner, fixes a name, or re-groups a synced channel loses those edits on the next Sync, persisted, with no prompt or diff. The body's "does not overwrite manual/CSV memories" is true and misses this class. Either skip fields the operator has touched (a dirty mask), stamp only on first insert, or warn.

3. The recallability repair heals partially, can be skipped entirely, and re-fires forever (inline). Three verified defects in one loop: (a) it keys on MemoryFields::isKnownMode, but the codec emits "CWL" and "WFM", neither in modes() — so CW-R/WFM rows poisoned by the first build stay display-only while their neighbours heal ("the fix mostly worked" bug reports); (b) it sits below if (!parsed.ok()) return;, so one non-fatal bad row anywhere in the document disables the whole repair; (c) it is un-versioned — it runs on every launch, so the moment DV/DD gain a neutral mapping (DSTR/FDV are already in modes()) every deliberately display-only row silently flips recallable, permanently, and no future codec decision can stick. The store's own kFormatVersion bump this PR makes is the migration hook the comment's "once" wants — gate the repair on storedRowVersion < 2, key it on the codec's rule, and move it into the store's schema layer.

4. The format-version bump is a one-way door for purely additive fields — and the repair walks users through it without any action of theirs. Every v2 field is read via value(...).toX(default), so a v1 build would parse a v2 document harmlessly; but flush() stamps kFormatVersion=2 on any save, load() refuses to write when stored > built, and the load-time repair itself triggers a flush. An operator who merely launches this build with synced rows and then rolls back finds an empty, read-only memory bank — indistinguishable from data loss, in the PR that exists because of an indistinguishable-from-data-loss bug. Don't bump the version for additive fields, or at minimum never auto-upgrade from load().

5. Tune half-applies then fails on a disconnected Icom. Imported rows always carry nativeFilter/offsetDir, so recall always reaches applyMemoryRecallDetails, which returns false without a session — after setMode/setFilterWidth/setFrequency have already run. The dialog is deliberately fully usable while disconnected, so this is a reachable ordinary path: half-applied slice plus a Tune error on a row presented as a normal database memory. Validate the backend requirement before mutating the slice.

6. Sync reports 100% success when zero rows were stored. The create-refusal and non-writable-bank paths are qCWarning-only, while the reply counter that feeds memoryRefreshFinished(true, n, n) increments before the delta is applied — an unreadable or full bank yields "297 of 297" and a success banner with nothing persisted. This is the exact silent-drop shape #5263 made loud on the command plane; thread a stored-count (or failure reason) back into the finished signal.

Also needing your ruling, not a blocker: persistsMemories = false for a radio that demonstrably persists and recalls its channels is in tension with Principle III's letter ("the deciding test is simply whether the radio can save and restore the value"). The mitigations are real — the client cannot write native memories, sync is one-directional, and both #5327 and the pre-#5283 design note prescribe this model — but III outranks the design doc, so the stale-recall trade (front-panel edit vs. un-resynced client row) should be ruled on explicitly rather than passed silently.

Nits (non-blocking, condensed)

• usesNativeMemorySchema is now compile-time impossible (Icom hardcodes persistsMemories=false; only Flex sets true), leaving ~150 lines of dead two-schema table code, a write-only memoryGroupColumnTitle capability, and — the real cost — the newly persisted channel/nativeFilter/dataMode/rxToneValue fields rendered nowhere: a split-tone repeater's RX tone is stored, un-viewable, un-editable.
• The Group-cell editor lost stored-group suggestions wherever hasProfiles is true — including the never-connected default-Flex state, where the dropdown is now empty for an operator with 40 grouped rows (typo-fork risk); and hasProfiles is the wrong predicate for "who owns the memory vocabulary" (the deleted code keyed on persistsMemories; MainWindow.cpp:7230 shows the !connected || form). The enable-check (case-insensitive, trimmed) and the backend accept-check (exact) also disagree, with the new gating test enshrining a string the backend rejects; and on IC-9700/MK2 a stored-group selection passes the GUI check and silently sweeps all groups.
• Import identity (importSource/importKey) is absent from the kv wire-codec and the CSV export, so a row round-tripped through either path loses its identity and the next Sync duplicates it — state the fields as backend→model-only in MemoryDelta.h or carry them explicitly.
• Sync-time UI cost compounds: per-row memoryChanged → 50 ms full-table rebuilds × up to five by-value RadioCapabilities constructions per rebuild ≈ thousands of 700-field builds per 297-channel sweep; the already-wired refresh-finished signal should own the single repopulate.
• The 47-byte IC-7300MK2 claim is live-radio fact with no docs/data/ evidence artifact (the repo's own IC-9700 precedent), layoutFor still says singleBytes=33 while the comment says 33 never occurs, and the split-length fact belongs in the layout table (the inline dialect != test is exactly how the bug being fixed got introduced); docs/architecture/radio-capabilities-map.md's canApplyMemories row still says split/RPS are display-only, which this PR's own test now falsifies.
• The identity-refusal guard — the advertised anti-aliasing protection — has no test reaching refreshMemories, while the PR's own radio_capability_gating_test pattern proves backend refusals are testable pre-transport; smaller: radioIdHex hand-rolls hex where QByteArray::toHex() is the idiom two files away, imports insert twice via the command-string round-trip, flush() pretty-prints then re-parses the whole bank, and the repair logs "repaired N" even when the flush was refused.

What was verified vs read

• Verified by me in primary sources: the old vs new recallable rules and the absent second-frequency field; the default:→simplex mapping; the owner/group/name stamps and their unconditional application; modes() vs the codec's "CWL"/"WFM"; the repair's position below the parsed.ok() early-return; persistsMemories=false with Flex as the only true producer and both renderers of the new fields behind the dead gate; main's editor-suggestion union vs the new spec.
• Refuted along the way (claims the pass itself killed): the radioId ordering race, the 297×load()/O(n²)-flush cost (loads latch, saves debounce), stale-Flex-rows leaking into imports, and my own initial "repair runs per command" reading (once per process).
• Not run: no hardware, no bridge session of my own — the author's live IC-7300MK2 bridge run covers the happy paths well; every blocker above lives in the paths that run's coverage-limits section names as untested.

Issue fit: the revised PR addresses #5327 by restoring the durable, editable client bank for Icom and making explicit native Sync an idempotent import. The prior split/RPS, annotation, normalization and load-repair blockers are corrected. The schema boundary is now limited to saves containing metadata that old writers cannot preserve safely; ordinary local banks remain version 1.

Everything in the final diff is explained by the issue and review corrections. No unrelated defaults, dependencies, protocol verbs or CHANGELOG entry were added. Current main's tuner assertions remain intact.

Blockers: none remaining in this review. Five addressed threads have concrete fix and regression evidence. Existing broader UI/CSV performance and native-model coverage notes remain non-blocking; the native metadata is not claimed to be fully editable or CSV-preserved.

Verified on source e87d783 (final head 8077d6b changes only the generated manifest), merged with current main a4227e6: full RADE-enabled ARM64 macOS app build; eight focused socket-free CTests; strict engine boundary, registration, CI gate, manifest and whitespace checks. Five independent mutations were rejected by the tests: split/RPS guard removal, unnormalized lookup, annotation overwrite, unconditional version 2, and load-time repair/write.

Runtime proof used only isolated offscreen DEMO-0001, with TX pinned off. The memory dialog displayed manual/imported/display-only rows and enabled local Add/Import/Remove. Normal GUI recall returned slice 0 at {frequency:14.25, mode:USB} and {frequency:7.05, mode:CW}; selecting the split fixture disabled Tune. The imported recall and disabled Tune survived closing/relaunching the process. Both review instances were closed. QRhi spectrum rendering is unavailable offscreen; no spectrum pixel or live Icom firmware convergence claim is made.

I tried to break slot collision handling, repeated/normalized Sync, empty-channel removal, cross-radio identity, locally edited annotations, restart before cache publication, safe recall classification and version-1 compatibility. The corrected code and tests held. No separate automated review skill was available; this includes direct issue-fit and lifecycle/failure-path review. Approval applies to 8077d6b; merge remains subject to current CI and repository protections.

Issue fit

The durable Icom working-bank fix now includes the reviewed editing, failure-reporting and disconnected-recall edges. Reviewed 43f5493 against current main d58e2b8a94af82f273f60010df17b080e6b2d907. No remaining merge-blocking finding in this pass.

Scope

Everything in the PR diff is explained by #5327. No unrelated dependencies, default changes, native-memory writes, TX or changelog entry.

Blocker disposition

1. Split/RPS/DV/DD remain display-only; the unsafe broadened recall rule is absent.
2. Repeat Sync preserves operator annotations.
3. The speculative load-time recallability repair remains removed; only explicit Sync refreshes native safety metadata.
4. Ordinary local banks remain schema 1; native/safety fields intentionally require schema 2, documented in the PR body.
5. Native recall now refuses before slice changes while disconnected; failure propagates through the memory-command callback. The dialog states the connection requirement.
6. Full/read-only/foreign-write failures cannot report successful Sync. The backend publishes its last occupied or empty-channel delta before completion, and the model checks the synchronous bank commit.
7. Local mode edits no longer retain a contradictory native DATA flag.
8. Current-main conflicts were resolved without resurrecting the broad test removed in #5452. PR-specific capability/filter assertions remain in registered socket-free targets. Offline stored-group suggestions were restored.

Evidence and limits

The ARM64 app build and eight focused CTests passed. New edit/save tests failed before the fixes. Deliberate mutations of final-reply ordering, the disconnected-recall guard and offline filter selection failed; the production fixes were restored, rebuilt and retested. Strict engine-boundary, test-registration, frozen CI-gate, manifest and whitespace checks passed.

The isolated offscreen demo read back DEMO-0001, model AetherSDR Demo, one slice/pan and transmitting=false; imported recall returned frequency=7.05, mode=CW. Before connecting, native Tune was disabled with its connection explanation and Local group remained selectable. Display-only Tune stayed disabled. Saved rows survived process restart; the review instance was stopped.

No live Icom firmware or TX validation was performed. Demo/control and socket-free model/codec evidence does not claim live firmware convergence. Existing native-metadata presentation/CSV-provenance limitations remain non-blocking follow-up scope, not newly delivered UI features.